-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bundle new collection of Header and Footer block patterns #43157
Conversation
Thanks for handling this so quickly! The new patterns look and work well in my testing. I have a couple comments:
Even though I'm not sure this is the right place to address this, I have some thoughts on the browsing and categorisation of the patterns:
|
When I tried to add author patterns, Kjellr told me the core patterns should not be in Gutenberg, but in the pattern directory. It would be good to establish what is the correct way. |
'content' => '<!-- wp:group {"align":"full"} --> | ||
<div class="wp-block-group alignfull"><!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"4em","bottom":"2em"}}},"layout":{"inherit":false}} --> | ||
<div class="wp-block-group alignfull" style="padding-top:4em;padding-bottom:2em"><!-- wp:paragraph {"align":"center"} --> | ||
<p class="has-text-align-center">Proudly Powered by <a href="https://wordpress.org">WordPress</a></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are using PHP in the block pattern, and the translation function is available, I think it would be good idea to make this text and the link translatable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, as well as in the other patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0836d91. What do you think?
lib/compat/wordpress-6.1/block-patterns/footer-with-background-color-and-three-columns.php
Outdated
Show resolved
Hide resolved
<div class="wp-block-group alignfull" style="padding-top:30px;padding-right:30px;padding-bottom:30px;padding-left:30px;"><!-- wp:site-title {"textAlign":"center","fontSize":"large"} /--> | ||
|
||
<!-- wp:paragraph {"align":"center"} --> | ||
<p class="has-text-align-center">Proudly Powered by <a href="https://wordpress.org">WordPress</a></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why the data-type="URL"
and data-id
are used inconsistently, on only some external links?
Does the pattern benefit from these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to @beafialho. If there are no specific reasons, we should just be consistent and boring. I am not familiar with this data-type
/ data-id
practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the liberty of removing those attributes along with other i18n changes in 0836d91
'content' => '<!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"30px","right":"30px","bottom":"30px","left":"30px"}}},"layout":{"type":"flex","flexWrap":"nowrap","justifyContent":"space-between"}} --> | ||
<div class="wp-block-group alignfull" style="padding-top:30px;padding-right:30px;padding-bottom:30px;padding-left:30px;"><!-- wp:site-title {"style":{"typography":{"fontStyle":"normal","fontWeight":"700"}},"fontSize":"large"} /--> | ||
|
||
<!-- wp:navigation {"ref":31,"layout":{"type":"flex","justifyContent":"space-between"},"style":{"spacing":{"blockGap":"30px"}},"fontSize":"large"} /--></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the practice is to remove the ref -There might not be a navigation menu with the ID "31" on the install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these aren't meant to be here (see "Issues" in the PR description).
<!-- wp:site-title {"style":{"elements":{"link":{"color":{"text":"var:preset|color|background"}}}}} /--></div> | ||
<!-- /wp:group --> | ||
|
||
<!-- wp:navigation {"ref":31,"__unstableLocation":"primary","layout":{"type":"flex","setCascadingProperties":true,"justifyContent":"right"}} /--></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only pattern that has an "__unstableLocation"
for the navigation block? and should the patterns use it? I believe it is still experimental, considering it is still called "unstable" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as in the other comment: this Nav block should be rewritten without ref and site-dependent attributes.
|
||
<!-- wp:site-tagline {"textAlign":"center"} /--> | ||
|
||
<!-- wp:spacer {"height":8} --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the spacer block needs a unit in the height attribute to be valid.
<!-- wp:spacer {"height":"8px"} -->
Was this removed on purpose? I suppose it depends what version of Gutenberg/Core that the pattern needs to support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye. I bet that was accidental.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed a few of these in 453c77f
'title' => _x( 'Footer with Large Font Size', 'Block pattern title' ), | ||
'blockTypes' => array( 'core/template-part/footer' ), | ||
'categories' => array( 'footer' ), | ||
'content' => '<!-- wp:group {"align":"full","style":{"spacing":{"blockGap":"15px","padding":{"top":"30px","right":"30px","bottom":"30px","left":"30px"}}},"layout":{"type":"flex","orientation":"vertical","justifyContent":"left"}} --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker of course, but this inconsistency of some patterns using em and this pattern using px for the spacing is a bit strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to @beafialho again. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually work with px. Can we apply the equivalent em values?
Correct. Inclusion in the Pattern Directory was also the goal for this collection. However, the Directory's pattern editor is not yet able to handle several site blocks, as they affect site options and other privileged data. If we wait for that support in the directory, we will miss the window for WP 6.1. So, after a discussion with Bea, and due to the beneficial impact that these patterns could have for users in 6.1, @mtias recommended that we include these patterns even if it means bundling in Gutenberg+Core. |
lib/compat/wordpress-6.1/block-patterns/centered-logo-in-navigation.php
Outdated
Show resolved
Hide resolved
Done in 5f18442
Hm, I don't see it. Or rather: the markup for each icon is missing from the pattern, but this is because the Social Link block is dynamic. On the other hand, the Social Links (plural) block is purely static. But you should be seeing the icons properly rendered in the editor and previews. Are you not?
Which ones do you see as not quite headers? (Also, feel free to make changes directly!)
I'm not sure what you mean: are you suggesting adding a new category that gets registered by the TT2 theme? Or actually in Core? Is the issue that patterns from the bundle cannibalise TT2 patterns or vice-versa? |
No, I don't see the Social Links (plural) block at all. But if you see them, it's probably something with the site I'm using to test it.
These:
I would make the changes myself but I don't have the technical knowledge to do so.
I'm thinking it would be nice to see the TT2 patterns together in its own category, not mixed with the other headers and footers. But I realize that can be confusing for some users so we can leave them as they are now. I don't feel strongly about it. |
Ah, but those aren't part of the PR. :) |
@beafialho: I haven't fixed these because I wanted your input. I could systematically replace them with something else, but usage is inconsistent across the remaining patterns:
The former seems more desirable to showcase the pattern, while the latter might be more ergonomic if users expect the editor to populate the menu with meaningful data. There is a lot of movement around the navigation blocks (e.g. how to best support theme switches, how to reconcile static and dynamic menus) that I haven't followed, and someone in the know might have better recommendations. |
@ryelle: could you possibly help us get this image asset uploaded in https://headerandfooterpatterns.files.wordpress.com/2022/01/5eca6-header-mountains-scaled-1.jpg |
Sure, here you go: https://s.w.org/patterns/files/2022/08/5eca6-header-mountains-scaled-1.jpg |
Thanks! Pushed! |
Patterns that aim to display whatever is the user's main menu should leave an empty navigation block (no id, no inner blocks); patterns that have a very specific layout should use Link Item at will with whatever placeholders make the most individual sense. |
And that will continue to be true for any new block, and the process needs to be established and documented. |
lib/compat/wordpress-6.1/block-patterns/footer-with-background-color-and-three-columns.php
Outdated
Show resolved
Hide resolved
What I ended up doing was rewriting all Navigation blocks as empty, while keeping any layout attributes, but discarding
FYI, the current UX for empty Navigation blocks could be better, but it's being iterated on by other contributors with WP 6.1 as the target. This also means that, for the time being, previews are less useful if the pattern contains an empty Navigation block. I think this should be good to go from a technical standpoint. The patterns themselves should be good enough, too, but I'd like a fresh set of eyes to actually test them all. Is anything else needed here, or will we then be able to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get it in, we'd want to test and refine until we can narrow down a set that makes sense to offer for headers and footers out of the box
Wonderful. FYI, per our private conversation I removed Waiting for the lights to go green, then 🚢 |
Exceptions: - logo-navigation-and-social-icons.php, whose Navigation block combines Page List with Social Icons - centered-logo-in-navigation.php, whose Navigation block arranges a Site Logo in the middle of Navigation Link blocks.
It can be added back in once it is rewritten in such a way that the Social Icons block is not a child of Navigation, but its direct sibling, while preserving the right-aligned design.
0d17e05
to
a641f63
Compare
Thanks for all the feedback, everyone! |
Based on conversations in #44843, I'm bumping this to WP 6.2. I will follow up with PR to update the compat directory for these files so changes aren't missed during the 6.2 release cycle. |
What?
Authored by @beafialho and @kjellr, these block patterns represent a consolidation of recurring headers and footers, simplified in order to work well across themes.
As they go hand in hand with the new site-editing features rolling out this year, the goal is to bundle these in WordPress 6.1 soon, after testing in Gutenberg.
Props beafialho, kjellr
Testing Instructions
viewportWidth
value).Issues
s.w.org
ref
, resulting in a broken pattern:centered-header
→centered-logo-in-navigation
and vice versasample-page
→simple-header